Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Orderbook: Add GetDepth to Base #1328

Merged
merged 3 commits into from
Aug 29, 2023
Merged

Orderbook: Add GetDepth to Base #1328

merged 3 commits into from
Aug 29, 2023

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented Aug 26, 2023

Base.GetDepth returns the concrete book of which Base is a copy This is probably useful for immutably monitoring orderbook health and state whereas FetchOrderbook would trigger a refresh.

Type of change

  • New feature (non-breaking change which adds functionality)

How has this been tested

  • go test ./... -race
  • golangci-lint run

@gbjk
Orderbook: Add GetDepth to Base
de5e92f 
Base.GetDepth returns the concrete book of which Base is a copy
This is probably useful for immutably monitoring orderbook health and state
whereas FetchOrderbook would trigger a refresh.
@gloriousCode gloriousCode requested a review from a team August 27, 2023 21:29
@gloriousCode gloriousCode added the review me This pull request is ready for review label Aug 27, 2023
gloriousCode
gloriousCode requested changes Aug 27, 2023
View reviewed changes
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Base already has access to the fields to make the call for GetDepth, but this is still a nice enough helper to avoid needing to provide params anyway.

exchanges/orderbook/orderbook.go Outdated Show resolved Hide resolved
shazbert
shazbert requested changes Aug 28, 2023
View reviewed changes
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit.

exchanges/orderbook/orderbook.go Show resolved Hide resolved
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 28, 2023

@gloriousCode

Base already has access to the fields to make the call for GetDepth, but this is still a nice enough helper to avoid needing to provide params anyway.

Base does, but nothing outside it does, right?
In the case of a consumer of the Bitfinex exchange, calling FetchOrderbook formats the pair, and then fetches the orderbook.
You can't call GetDepth without knowing the pair, and you can't find out if the orderbook is valid without risking a fetch.
There's a few other options, but this felt easiest.

@gbjk
Copy link
Collaborator Author

gbjk commented Aug 28, 2023

All comments addressed.

We discussed why this method is helpful, and with some refactoring/improvement/something in bitfinex maybe we could get away with calling FormatExchangePair before calling Fetch ever, or something.
But it's a can of worms so for now this solves my need and isn't too code-rotty.

@gbjk gbjk requested review from shazbert and gloriousCode August 28, 2023 03:50
@gbjk gbjk force-pushed the feature/orderbook_depth branch from 7c66c07 to 772bfb6 Compare August 28, 2023 04:50
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 28, 2023

Ah. This is failing when run as a whole suite. Will work out why.
Probably service is populated by prev test

@gbjk gbjk force-pushed the feature/orderbook_depth branch from 772bfb6 to 7267545 Compare August 28, 2023 04:53
@gbjk
Copy link
Collaborator Author

gbjk commented Aug 28, 2023

Yep. It was that. Fixed.

@gbjk gbjk force-pushed the feature/orderbook_depth branch from 7267545 to 0a1500a Compare August 28, 2023 04:57
shazbert
shazbert approved these changes Aug 29, 2023
View reviewed changes
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK. Thanks.

gloriousCode
gloriousCode approved these changes Aug 29, 2023
View reviewed changes
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK! Thanks for making those changes

@thrasher- thrasher- merged commit 9a0f261 into thrasher-corp:master Aug 29, 2023
@gbjk gbjk deleted the feature/orderbook_depth branch August 29, 2023 06:45
shazbert pushed a commit to shazbert/gocryptotrader that referenced this pull request Nov 10, 2023
@gbjk
Orderbook: Add GetDepth to Base (thrasher-corp#1328)
59596de 
* Orderbook: Add GetDepth to Base

Base.GetDepth returns the concrete book of which Base is a copy
This is probably useful for immutably monitoring orderbook health and state
whereas FetchOrderbook would trigger a refresh.

* Orderbook: Reword GetDepth comment

* Orderbook: Add test for Base.GetDepth
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shazbert shazbert shazbert approved these changes

@gloriousCode gloriousCode gloriousCode approved these changes

Assignees

@gbjk gbjk

Labels
review me This pull request is ready for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gbjk @gloriousCode @shazbert @thrasher-